-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xds/bootstrap: add trusted_xds_server server feature
#8692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
ab07959 to
c691084
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8692 +/- ##
==========================================
- Coverage 83.42% 83.29% -0.14%
==========================================
Files 416 419 +3
Lines 32303 32438 +135
==========================================
+ Hits 26948 27018 +70
- Misses 3993 4042 +49
- Partials 1362 1378 +16
🚀 New features to boost your workflow:
|
trusted_xds_server server feature
|
The generic xDS client's configuration can be found here:
We need to propagate this newly added server feature to the generic xDS client as well. In order to do that, we need to do the following:
func (s *ServerConfig) SupportsServerFeature(feature ServerFeature) bool { ... }
type ServerFeature int
const (
ServerFeatureIgnoreResourceDeletion ServerFeature = iota
ServerFeatureTrustedXDSServer
)
|
|
@Pranjali-2501 : Please assign it back to me once you've handled the comment. Thanks. |
easwars
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo minor nits
|
|
||
| // IgnoreResourceDeletion is a server feature which if set to true, | ||
| // indicates that resource deletion errors from xDS management servers can | ||
| // be ignored and cached resource data can be used. | ||
| // | ||
| // This will be removed in the future once we implement gRFC A88 | ||
| // and two new fields FailOnDataErrors and | ||
| // ResourceTimerIsTransientError will be introduced. | ||
| IgnoreResourceDeletion bool | ||
|
|
||
| // TODO: Link to gRFC A88 | ||
| ServerFeature ServerFeature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You could get rid of the newline and add a trailing comment to this line saying this field stores a bitmap of supported features.
| serverFeatures = serverFeatures | xdsclient.ServerFeatureTrustedXDSServer | ||
| } | ||
| gsc := xdsclient.ServerConfig{ | ||
| ServerIdentifier: clients.ServerIdentifier{ServerURI: sc.ServerURI(), Extensions: grpctransport.ServerIdentifierExtension{ConfigName: sc.SelectedChannelCreds().Type}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please use a separate line for individual fields (in sub-struct fields as well). I understand this is continuing to do what the existing code was doing. But that happened to be a part of a giant refactor, and it was often easy to miss things during the review. See: go/go-style/decisions#literal-formatting
Thanks.
This PR implements the Bootstrap config changes for gRFC A81.
Authority rewriting is a security-sensitive feature that should only be enabled when the xDS server is explicitly trusted to provide such configuration. gRFC A81 specifies that this trust is indicated by adding
trusted_xds_serverto the server_features list for a given server in the bootstrap file.RELEASE NOTES: None